Apply checkout.fetch-depth to safe_outputs checkout steps#33746
Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
checkout.fetch-depth to safe_outputs checkout steps
…out-settings # Conflicts: # .github/workflows/daily-cache-strategy-analyzer.lock.yml # .github/workflows/daily-fact.lock.yml # .github/workflows/daily-observability-report.lock.yml # .github/workflows/duplicate-code-detector.lock.yml # .github/workflows/grumpy-reviewer.lock.yml # .github/workflows/issue-arborist.lock.yml # .github/workflows/necromancer.lock.yml # .github/workflows/schema-feature-coverage.lock.yml # .github/workflows/smoke-call-workflow.lock.yml # .github/workflows/smoke-codex.lock.yml
There was a problem hiding this comment.
Pull request overview
This PR fixes safe_outputs checkout step generation so it honors the user-configured default checkout.fetch-depth (e.g., 0 for full history), matching behavior already supported in the agent job and unblocking create_pull_request flows that require commit history.
Changes:
- Updated
buildSharedPRCheckoutStepsto derivefetch-depthfrom the default checkout override inCheckoutConfigs(falling back to1). - Applied the configured
fetch-depthto bothsafe_outputsPR checkout paths (comment-triggered trusted default-branch checkout and the standard non-comment checkout). - Added a unit test asserting
fetch-depth: 0is emitted when configured, and updated lock workflows accordingly.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_safe_outputs_steps.go | Reads default checkout override to propagate configured fetch-depth into safe_outputs checkout steps. |
| pkg/workflow/compiler_safe_outputs_steps_test.go | Adds coverage ensuring configured fetch-depth (0) is reflected in generated safe_outputs YAML. |
| .github/workflows/pr-sous-chef.lock.yml | Updates generated safe_outputs checkout steps to use fetch-depth: 0 per config. |
| .github/workflows/glossary-maintainer.lock.yml | Updates generated safe_outputs checkout steps to use fetch-depth: 0 per config. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
| fetchDepth = *defaultCheckout.fetchDepth | ||
| consolidatedSafeOutputsStepsLog.Printf("Using custom checkout fetch-depth for safe_outputs: %d", fetchDepth) |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed for PR #33746: missing the implementation label and only 29 new lines in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Quality Review Summary
Verdict: ✅ Approved
Analysis: This is a clean, focused bug fix that applies checkout.fetch-depth configuration to safe_outputs checkout steps. The implementation follows good practices:
- Proper nil safety: Correct nil checks before dereferencing pointers
- Consistent application:
fetch-depthapplied to both checkout paths (default branch + standard) - Appropriate logging: Debug output for visibility during compilation
- Targeted test coverage: Focused test validates the core behavior
Note: One existing review comment correctly identifies the need for fetch-depth validation (negative values). This is the primary code quality concern and has already been raised.
No additional code quality issues found. The implementation is solid and the changes are appropriate for the bug being fixed.
🔎 Code quality review by PR Code Quality Reviewer · ● 523.2K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd based on the bug fix nature of this PR.
Key Themes
- Root cause addressed: The fix correctly propagates
checkout.fetch-depthfrom frontmatter config tosafe_outputscheckout steps, solving the immediate problem. - Test coverage: Added a focused test for the happy path (
fetch-depth: 0), but missing edge case tests for nil/emptyCheckoutConfigsscenarios. - Observability: Good use of debug logging when custom depth is applied, but missing log for default fallback case.
Positive Highlights
- ✅ Backward compatible: Defaults to
fetch-depth: 1when no override is configured - ✅ Consistent application: Both checkout paths (comment-triggered and standard) use the same depth logic
- ✅ Clear PR description: Excellent summary of the problem, scope, and behavioral impact
- ✅ Minimal changeset: Surgical fix that doesn't touch unrelated code
Suggestions
- [/tdd] Add edge case tests for nil/empty
CheckoutConfigsto validate the fallback behavior - [/diagnose] Add debug log when using default
fetch-depth: 1(helps troubleshoot config issues) - [/diagnose] Consider extracting
NewCheckoutManagercall to a variable for readability
Verdict
This is a solid bug fix with good test coverage for the main scenario. The suggestions are minor improvements that would strengthen the regression protection and observability. Approving with optional improvements noted inline.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 557.9K
| {FetchDepth: &fetchDepthZero}, | ||
| }, | ||
| checkContains: []string{ | ||
| "fetch-depth: 0", |
There was a problem hiding this comment.
[/tdd] This test case validates the happy path (custom fetch-depth is applied), but it's missing edge case coverage:
- What happens when
CheckoutConfigsis nil? - What happens when
CheckoutConfigsis empty ([])? - What happens when
FetchDepthpointer is nil?
Consider adding test cases for these scenarios to ensure the default fallback logic (fetchDepth := 1) is exercised:
{
name: "uses default fetch-depth when CheckoutConfigs is nil",
safeOutputs: &SafeOutputsConfig{
CreatePullRequests: &CreatePullRequestsConfig{},
},
checkoutConfigs: nil,
checkContains: []string{
"fetch-depth: 1",
},
},
{
name: "uses default fetch-depth when CheckoutConfigs is empty",
safeOutputs: &SafeOutputsConfig{
CreatePullRequests: &CreatePullRequestsConfig{},
},
checkoutConfigs: []*CheckoutConfig{},
checkContains: []string{
"fetch-depth: 1",
},
},These edge cases protect against regressions if the NewCheckoutManager logic changes.
| func (c *Compiler) buildSharedPRCheckoutSteps(data *WorkflowData) []string { | ||
| consolidatedSafeOutputsStepsLog.Print("Building shared PR checkout steps") | ||
| var steps []string | ||
| fetchDepth := 1 |
There was a problem hiding this comment.
[/diagnose] Potential inefficiency: NewCheckoutManager(data.CheckoutConfigs) is called inline within the if condition.
If CheckoutConfigs is large or NewCheckoutManager has non-trivial initialization logic, this could be optimized:
fetchDepth := 1
checkoutManager := NewCheckoutManager(data.CheckoutConfigs)
if defaultCheckout := checkoutManager.GetDefaultCheckoutOverride(); defaultCheckout != nil && defaultCheckout.fetchDepth != nil {
fetchDepth = *defaultCheckout.fetchDepth
consolidatedSafeOutputsStepsLog.Printf("Using custom checkout fetch-depth for safe_outputs: %d", fetchDepth)
}This makes the code more readable and avoids potential repeated instantiation if the manager is used elsewhere in this function.
| var steps []string | ||
| fetchDepth := 1 | ||
|
|
||
| if defaultCheckout := NewCheckoutManager(data.CheckoutConfigs).GetDefaultCheckoutOverride(); defaultCheckout != nil && defaultCheckout.fetchDepth != nil { |
There was a problem hiding this comment.
[/diagnose] Good defensive programming: initializing fetchDepth to 1 maintains backward compatibility if the lookup fails.
Consider adding a debug log when using the default:
fetchDepth := 1
if defaultCheckout := NewCheckoutManager(data.CheckoutConfigs).GetDefaultCheckoutOverride(); defaultCheckout != nil && defaultCheckout.fetchDepth != nil {
fetchDepth = *defaultCheckout.fetchDepth
consolidatedSafeOutputsStepsLog.Printf("Using custom checkout fetch-depth for safe_outputs: %d", fetchDepth)
} else {
consolidatedSafeOutputsStepsLog.Print("Using default fetch-depth: 1")
}This helps with debugging when users expect depth=0 but it silently falls back to depth=1.
🧪 Test Quality Sentinel ReportTest Quality Score: 60/100
Test Classification Details
📊 Detailed AnalysisTest Case: "uses custom checkout fetch-depth"What this test verifies:
Assertions (3 total):
Behavioral contract tested: Why this is a design test (high value):
Test structure:
|
|
@copilot review all comments, lint go |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I reviewed the comments and addressed the fetch-depth validation gap by rejecting negative Included in commit |
checkoutoverrides were only applied in the agent job, whilesafe_outputsalways checked out withfetch-depth: 1. This brokecreate_pull_requestflows that require commit history when users configure full checkout (fetch-depth: 0).Scope of fix: safe_outputs shared PR checkout generation
buildSharedPRCheckoutStepsto read the default checkout override fromCheckoutConfigs.safe_outputscheckout steps now use configuredfetch-depthwhen present, otherwise keep the default (1).Behavioral impact
checkout.fetch-depthis now honored across both the agent job andsafe_outputs, removing the need to post-process compiled lock workflows.Targeted coverage
fetch-depth: 0) propagates into generatedsafe_outputscheckout YAML.